-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix selected parent category is not displayed as selected #37805
fix selected parent category is not displayed as selected #37805
Conversation
@hoangzinh Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
@@ -1051,7 +1051,7 @@ function getCategoryListSections( | |||
title: Localize.translateLocal('common.all'), | |||
shouldShow: true, | |||
indexOffset, | |||
data: getCategoryOptionTree(filteredCategories), | |||
data: getCategoryOptionTree(filteredCategories, false, selectedOptionNames), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dukenv0307 should we update other places calling to this method as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you point out any similar cases that we need to fix? I am worried that adding the change to other places can leads to the regression
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, these are several cases:
Case 1: Search a category
App/src/libs/OptionsListUtils.ts
Lines 984 to 996 in 79d6cda
if (searchInputValue) { | |
const searchCategories = enabledCategories.filter((category) => category.name.toLowerCase().includes(searchInputValue.toLowerCase())); | |
categorySections.push({ | |
// "Search" section | |
title: '', | |
shouldShow: true, | |
indexOffset, | |
data: getCategoryOptionTree(searchCategories, true), | |
}); | |
return categorySections; | |
} |
Case 2: When there are no categories
App/src/libs/OptionsListUtils.ts
Lines 972 to 982 in 79d6cda
if (numberOfCategories === 0 && selectedOptions.length > 0) { | |
categorySections.push({ | |
// "Selected" section | |
title: '', | |
shouldShow: false, | |
indexOffset, | |
data: getCategoryOptionTree(selectedOptions, true), | |
}); | |
return categorySections; | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- In case 1: Search a category
It will be handled in #37479
- In case 2: When there are no categories:
I cannot reproduce this case because, if there is no category, the category picker screen will display as "Hmm... it's not here"
@dukenv0307 it looks like we miss Android native recording |
@hoangzinh I updated android native video |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2024-03-11.at.22.18.47.android.movMacOS: Chrome / SafariScreen.Recording.2024-03-11.at.21.59.56.web.mov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@blimpich looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Author checklist was slightly off, not an emergency. |
🚀 Deployed to staging by https://github.com/blimpich in version: 1.4.51-0 🚀
|
🚀 Deployed to production by https://github.com/luacmartins in version: 1.4.51-3 🚀
|
Details
Fixed Issues
$ #37326
PROPOSAL: #37326 (comment)
Tests
Precondition:
Test
Test: Test 1
Test: Test 2
Offline tests
QA Steps
Precondition:
Test
Test: Test 1
Test: Test 2
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Screencast.from.07-03-2024.06.46.34.webm
Android: mWeb Chrome
Screencast.from.06-03-2024.07.16.25.webm
iOS: Native
Screencast.from.06-03-2024.07.23.08.webm
iOS: mWeb Safari
Screencast.from.06-03-2024.07.11.45.webm
MacOS: Chrome / Safari
Screencast.from.06-03-2024.07.02.55.webm
MacOS: Desktop
Screencast.from.06-03-2024.07.15.04.webm